Add a new protected method DefaultCrudRepository.ensurePersistable.
( I didn't use name fromEntity cause I get confused easily with the name while doing the task..)

+1 to find a better name than fromEntity.

Originally, I wanted to introduce fromEntity as something that's useful beyond validation of navigational properties, it has the potential to serve as a solution for LB3 hook persist.

The idea is to have a protected Repository method that's called whenever a CRUD method needs to convert data supplied by the user to data passed down to juggler. This new method can modify the data (e.g. by returning a new data object), but also perform validations (e.g. reject invalid data by throwing an error).

It makes me wonder, is this something we should aim for as part of this work?

@agnes512 The added tests LGTM in general(I will do a line by line review later), left two comments about the design of ensurePersistable:

#4148 (comment) feel free to improve it in a separate story or at least a separate PR

#4148 (comment) can be done in a separate story.

Originally, I wanted to introduce fromEntity as something that's useful beyond validation of navigational properties, it has the potential to serve as a solution for LB3 hook persist.

@bajtos Could you elaborate more about ^? are you looking for something like an ensurePersistant interceptor? my related comment #4148 (comment)

Originally, I wanted to introduce fromEntity as something that's useful beyond validation of navigational properties, it has the potential to serve as a solution for LB3 hook persist.

Could you elaborate more about ^? are you looking for something like an ensurePersistant interceptor? my related comment #4148 (comment)

The use case I have in mind is storing certain properties as encrypted in the database, see #2095.

• Inside toEntity method, we can decrypt the value fetched from the database and store plain-text value in the Entity property.
• Inside fromEntity method, we can encrypt the plain-text value stored in the Entity property and send encrypted value in the "raw data" sent to to underlying DAO/connector methods.

@bajtos Thank you for the explanation. I am looking at your original proposal as

class DefaultCrudRepository<T, ID> /*...*/{
  protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    // "persist hook" - no-op by default
    return entity;
  }

  protected async dataToEntity(data: DataObject<T>, options?: Options): DataObject<T> {
    // "load hook" - no-op by default
    return this.toEntity(data);
  }

  async create(entity: DataObject<T>, options?: Options): Promise<T> {
    const data = await this.entityToData(entity, options);
    const model = await ensurePromise(this.modelClass.create(data, options));
    const result = await this.dataToEntity(model);
    return result;
  }

  // etc.
}

Do you mean we implement no-op entityToData and dataToEntity in DefaultCrudRepository then add Agnes's navigation property check in generated Repositories by CLI?

// Check navigation properties
// This check will be added in EACH created Repository that extends `DefaultCRUDRepository`
// as the built-in implementation for `entityToData`
// If developer wants to custom the logic how would they do it? Create a custom
// `BaseCRUDRepository` with their implementations of `entityToData` and `dataToEntity`, 
// then make the created Repositories extend the `BaseCRUDRepository`?

protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    const data: AnyObject = new this.entityClass(entity);

    const def = this.entityClass.definition;
    for (const r in def.relations) {
      const relName = def.relations[r].name;
      if (relName in data) {
        throw new Error(
          `Navigational properties are not allowed in model data (model "${this.entityClass.modelName}" property "${relName}")`,
        );
      }
    }
}

Do you mean we implement no-op entityToData and dataToEntity in DefaultCrudRepository then add Agnes's navigation property check in generated Repositories by CLI?

I don't think the navigation property check belongs to generated repositories, IMO this check should be executed by DefaultCrudRepository.

When I was writing those comments for entityToData and dataToEntity, I was not aware that these methods can be useful for checking the presence of navigational properties.

I think we should change the default implementation of entityToData from no-op to a check rejecting navigational properties. Something along the following lines:

class DefaultCrudRepository<T, ID> /*...*/{
  // "persist hook"
  protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    ensurePersistable(entity, options);
    return entity;
  }

  // ...
}

Please note the name entityToData may not be the best one, especially considering that we already have toEntity method.

class DefaultCrudRepository<T, ID> /*...*/{
  // "persist hook"
  protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    ensurePersistable(entity, options);
    return entity;
  }

  // ...
}

Cool thank you 👍 now I understand your plan

This is tricky, I'll need to find more time to look into the issue in more depth.

My initial feeling is that we are trying to work around a flaw in the way how repository code and the shared test suite is designed. Instead of adding more and more workarounds, I would prefer to find a better design that will not require these hacks.

in the ensurePersistable, the //FIXME from @bajtos says mongoDB would break replaceById with ObjectID issue if we apply ensurePersistable to replaceIdBy.
By debugging, I think it's caused by replacing the id project (ObjectID type) to a string type id. So in my proposal, I remove the id property from the target data for replaceById method in ensurePersistable since we don't need it anyway.

Ideally, when we run the test suite against MongoDB, all PK and FK values should be always strings. This is typically achieved by defining the properties as {type: 'string', mongodb: {dataType: 'ObjectID'}} (plus id, required and generated flags). Unfortunately, when a PK property is generated, LoopBack ignores the user-provided type and uses the type defined by the connector. We have recently added a flag to disable that behavior, see loopbackio/loopback-datasource-juggler#1783. What happens when we enable that flag in repository-tests models - will it solve the issues we are observing here?

The test case here skips Cloudant connector because the Customer model in fixtures doesn't have _rev property defined.

This is similar to the discussion we had in #3968, see #3968 (comment) IMO, we should find a way how to modify Customer model to include _rev property when testing Cloudant.

IMO, the current way of setting up test models in repository-tests is no longer supporting the needs of different connectors.

• MongoDB connector wants to set mongodb: {dataType: 'ObjectID'}.
• Cloudant wants to add _rev property
• SQL connectors want to use number as the type for primary & foreign keys. NoSQL connectors want to use string type.

Ideally, CrudFeatures should provide generic extension points allowing connectors to supply additional model/property configuration that will be applied by the shared test suite.

One of the problems I see in the current setup is that models used by relations are hard-coded in TypeScript source files (see e.g. packages/repository-tests/src/crud/relations/fixtures/models/address.model.ts). This makes it difficult to create a model that's tailored to the connector being used. See for example how we are using MixedIdType and modifying PK property metadata in the tests.

IMO, our life would be much easier if model classes were created dynamically by a factory function, as can be see in the non-relation tests - e.g. here:

https://github.com/strongloop/loopback-next/blob/1de6e122da3b27190dc8a08b739f0e484470d524/packages/repository-tests/src/crud/create-retrieve.suite.ts#L29-L53

Anyhow, I understand such change may require more effort that we want to invest right now. Even if we decide to do it, it belongs to a new pull request, not here.

In that light, let's try to find a way how to patch the current test setup so that we don't have to implement workarounds in the production code, and instead have the test models use the right model settings & property definitions, ones that our users should apply too.

I think that means we need to add _rev property to the Customer model in the fixture. I am not happy to use _rev when testing SQL databases, but I am ok to accept it as a temporary workaround for the limitations of current test fixture setup. We also need to fix definition of PK/FK properties and/or enable useDefaultIdType, so that PK/FK properties are always converted to string when testing MongoDB. The last step is to open a follow-up issue to refactor the shared test suite to better support needs of different connectors.

Thoughts?

@bajtos Thanks for the review and very detailed suggestion!

We have recently added a flag to disable that behavior, see loopbackio/loopback-datasource-juggler#1783. What happens when we enable that flag in repository-tests models - will it solve the issues we are observing here?

I tried useDefaultIdType but not working, maybe I didn't update the juggler to include that fix, let me try again, and that's why I used the solution in the last commit as a workaround(while it turns out to be a wrong solution).

I think that means we need to add _rev property to the Customer model in the fixture.

👍 I updated in the 2nd last commit, and all tests pass EXCEPT the one failure in the mongodb test suite. The changes in the last commit are actually quite broken.

Hi there, I am trying to perform a crud operation (post and patch) without any navigational property, but the error shows off, I can't understand why...

To make it short, I have a simple model :

class Bag extends Entity {
@property() id;
@property() name;
@hasMany(() => Stuff)
  stuffs?: Array<Stuff> = [];
}

In my repository I do something like:

this.updateById(1, {name: 'new bag');

And I got this error : Error: Navigational properties are not allowed in model data (model "Bag" property "stuffs");

Am I missing something ?

--- EDIT ---
All right it was because of the assigment of stuffs in my class, I just had to remove "= []".

@CedricBad with the hasMany decorator,

@hasMany(() => Stuff)
  stuffs?: Array<Stuff>

if a Bag instance doesn't have any Stuff, stuffs would be empty and won't be included in the result if you travers bagRepository.find({include:[{relation: 'stuffs'}]}).

Please let me know if you have any questions.

@agnes512

I was looking for a way to have my stuffs always included, even if there is none. That is why I assigned an empty array to the stuffs. But then I have the error mentionned aboved, so I guess there is no way to have a default value assigned to a relationnal item.
Sorry if I'm not clear, I'm giving you my best english 💃